Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Portafly: Adding reacti18next #1784

Merged

Conversation

damianpm
Copy link
Contributor

@damianpm damianpm commented Mar 20, 2020

What this PR does / why we need it:

We need to choose a localization/internazionalization framework for portafly

This PR tests react-i18next

Pros:

  • Very complete, lots of options
  • Ready for react, includes:

Cons:
Requires more configuration, is more complex than polyglot

Which issue(s) this PR fixes

https://issues.redhat.com/browse/THREESCALE-4685 (parent)
https://issues.redhat.com/browse/THREESCALE-4687 (format of strings files)
https://issues.redhat.com/browse/THREESCALE-4688 (choosing library)

Verification steps

cd portafly/

yarn

SKIP_PREFLIGHT_CHECK=true PORT=3003 yarn start

@damianpm damianpm force-pushed the portafly/THREESCALE-4688-localization-framework-react-i18next branch 2 times, most recently from e96b235 to e84394c Compare March 20, 2020 09:01
@damianpm damianpm changed the title Adding reacti18next [POC] Portafly: Adding reacti18next Mar 20, 2020
@damianpm damianpm marked this pull request as ready for review March 20, 2020 09:24
@damianpm damianpm requested review from didierofrivia, josemigallas and a team March 20, 2020 09:26
@damianpm damianpm force-pushed the portafly/THREESCALE-4688-localization-framework-react-i18next branch from e84394c to 035459c Compare March 20, 2020 11:14
@damianpm
Copy link
Contributor Author

Comparing react-i18next with polyglot (see #1776, #1783)

react-i18next Polyglot
Complete framework, includes several dependencies Small library
Needs more configuration Simple configuration with default options, if we need to extend it is not simple anymore
Supports formatted strings (bold,italic, etc.) out of the box Format not supported, we need to implement it, see this issue
Includes a useTranslation() hook Not provided, we need to implement it, see this PR
Includes a <Trans /> component Not provided
Includes a <Translation/> component Not provided
Includes a withTranslation HOC Not provided

Resuming:
Polyglot is simpler, just works with strings, we need to add everything we need
Adding our own hooks (or custom components like <Trans/> in the future) means we need to maintain them, instead of only using them.
FYI @didierofrivia @josemigallas @thomasmaas @cntlsn

@josemigallas
Copy link
Contributor

I am 80% in favor of using Polyglot, mainly because it's simpler, smaller and hence easier to handle. As for the aforementioned features:

  • Hook: it was easy to implement and very easy to customize. It uses React built-in features directly which is also a plus.
  • Specialized components: I don't think we need anymore because of Hooks.
  • Formatting: a bit of a 🥒 but I am in favor of using HTML for hyperlinks and simple formatting (bold, italics).

@damianpm
Copy link
Contributor Author

damianpm commented Mar 23, 2020

About the hook, we can define it now and later we can forget about it, is not my main concern.

But think about the formatting, because this is going to be a daily need.
Imagine a text with complex format, we may end with something like this:

String in our json file:

"text_formatted": "%{strong_start}%{name}%{strong_start}, welcome to %{strong_start}%{place}%{strong_end}!, we support %{strong_start}bolded%{strong_end}, %{italic_start}italic%{italic_end} or %{strong_start}%{italic_start}%{more_text}%{italic_end}%{strong_end}."

And then use it in our components:

t('text_formatted', {
  strong_start: '<strong>',
  strong_end: '</strong>',
  italic_start: '<italic>',
  italic_end: '</italic>',
  name: 'Juan Doe',
  place: 'portafly',
  more_text: 'nested formatting'
 })

It will be rendered as plain text by polyglot:

<strong>Juan Doe<strong>, welcome to <strong>portafly</strong>!, we support <strong>bolded</strong>, <italic>italic</italic> or <strong><italic>nested formatting</italic></strong>.

Then, to parse it as HTML we need to use another lib like ReactHtmlParser:

ReactHtmlParser(
  t('text_formatted', {
    strong_start: '<strong>',
    strong_end: '</strong>',
    italic_start: '<italic>',
    italic_end: '</italic>',
    name: 'Juan Doe',
    place: 'portafly',
    more_text: 'nested formatting'
  })
)

Or, if we don't want to use ReactHtmlParser, then "make a middleware or plugin to support markdown or sanitize the HTML" as @josemigallas pointed in a comment in our chat

Also think what happens if we need to render jsx (e.g a link) in the text. react-i18next supports it, polyglot not.

Polyglot option is not so simple after all.

FYI @didierofrivia @josemigallas @thomasmaas @cntlsn

@josemigallas
Copy link
Contributor

It is worth pointing out how formatting would look like using the different libraries.

Welcome to *PaternFly*! The best app ever

  • i18next
// en.yml
title: "Welcome to {{patternfly, bold}}! The {{best, italic}} app ever"
title_best: "best"

// component.jsx
i18next.t('title', { patternfly: 'PatternFly', best: `${i18next.t('title_best')}` })

Docs team has to insert interpolation objects they don't understand and can't use full sentences.

  • reacti18next
// en.yml
title: "Welcome to <1>Patternfly</1>! The <3>best</3> app ever"

// component.jsx
<Trans i18nKey="title">
    Welcome to <strong>Patternfly</strong>! The <em>best</em> app ever
</Trans>

Docs team has to insert weird html-like elements that they also wouldn't understand.

  • polyglot
// en.yml
title: "Welcome to <strong>Patternfly</strong> The <em>best</em> app ever"

// component.jsx
polyglot.t('title')

Docs would need to use common HTML formatting tags.


You have 5 new messages. Mark all as read.

  • i18next
// en.yml
messages: "You have {{count}} new message. {{link}}",
messages_plural: "You have {{count}} new messages. {{link}}"
messages_interval: "(0){You have no new messages.};"
messages_mark_unread: "Mark it as read"
messages_mark_unread_plural: "Mark all as read"

// component.jsx
i18next.t('messages', {
  postProcess: 'interval',
  count: messages,
  link: `<a href="#">${i18next.t(messages_mark_unread, { count: messages })}</a>`
});

Interpolation gets more complex and Docs cannot handle links directly.

  • reacti18next
// en.yml

"messages": "You have {{count}} new message. <5>Mark all as read</5>.",
"messages_plural": "You have {{count}} new messages. <5>Mark all as read</5>."

// component.jsx
<Trans i18nKey="messages" count={messages}>
    You have {{messages}} new message. { messages > 0 && <Link to="#">Mar all as read</Link>}.
</Trans>

This doesn't look so bad, still Doc team cannot handle links, needs to use weird html-link tags.. and it seems like it doesn't support interval plurals (i.e. count: 0).

  • polyglot
// en.yml
messages: "You have %{smart_count} new message. <a href='%{link}'>Mark it as read</a> |||| You have %{smart_count} new messages. <a href='%{link}'>Mark all as read</a>"
messages_none: "You have no new messages."

// component.jsx
messages > 0
  ? polyglot.t('messages', { smart_count: messages, link: '#' })
  : polyglot.t('messages_none')

Need to use smart_count as a keyword and |||| to separate singular/plural phrases.


My conclusion is it isn't worth it to use a complex library such as i18next. I'm not sure at all it's going to pay off learning to use it and all the headaches we're going to have. If the only problem is docs team not wanting to use HTML in the phrases I think we should still go for Polyglot.

While doing this small experiment I spend 1 min tops writing translations for Polyglot, the rest of the hours was trying to figure out how to use i18next. Maybe it doesn't have rich features or we have to craft some small parses/functions ourselves but I don't think that's a problem at all.

@damianpm damianpm changed the title [POC] Portafly: Adding reacti18next [WIP] Portafly: Adding reacti18next Mar 26, 2020
@damianpm damianpm force-pushed the portafly/THREESCALE-4688-localization-framework-react-i18next branch 2 times, most recently from e3faa98 to cab40ee Compare March 27, 2020 09:18
@damianpm
Copy link
Contributor Author

@josemigallas @didierofrivia
I've clean the PR, removed all the demo code, must be ready now

@damianpm damianpm changed the title [WIP] Portafly: Adding reacti18next Portafly: Adding reacti18next Mar 27, 2020
portafly/src/i18n/locales/en/shared.json Show resolved Hide resolved
portafly/src/pages/Overview.tsx Outdated Show resolved Hide resolved
transKeepBasicHtmlNodesFor: ['br', 'strong', 'i']
},
resources: {
en: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to check it first but I think we can simplify this.

import * as Translations from 'i18n/locales'

const options = {
  resources: Translations
}

In order to prevent repetition, too many imports and a very big resources property. Each locale should have its own "index" file such as i18n/locales/EN, i18n/locales/JP, etc. And then in i18n/index we export an object with everything needed for options.resources:

// i18n/locales/index.ts
import EN from 'i18n/locales/EN'
import JP from 'i18n/locales/JP'

export {
  en: EN,
  jp: JP
}

// i18n/i18n.tsx
import * as Translations from 'i18n/locales'

// ...

const options = {
  // ...
  resources: Translations
}

format: formatFn,
escapeValue: false
},
ns: ['shared', 'overview', 'analytics', 'applications', 'integration'],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be a type for the "sections". And be used here as well as in locales/index.ts.

}

const options = {
lng: 'en',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally there would also be a type for languages, so that we can use it as keys for retrieving the translations, like in https://github.com/3scale/porta/pull/1783/files#diff-86aa4a15c591a31fa47c49447bc69228R10-R11:

export const TRANSLATIONS: Record<Locales.LOCALES, ITranslations> = {
  [Locales.enUS]: enUsTranslations,
  [Locales.jaJP]: jaJpTranslations
}

@damianpm damianpm force-pushed the portafly/THREESCALE-4688-localization-framework-react-i18next branch from de1bced to 3276742 Compare March 27, 2020 15:11
@damianpm damianpm force-pushed the portafly/THREESCALE-4688-localization-framework-react-i18next branch from 3276742 to d23ef52 Compare March 27, 2020 15:13
adds some types and encapsulates translations files
Copy link
Contributor

@josemigallas josemigallas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@damianpm damianpm merged commit 4663599 into master Apr 1, 2020
@josemigallas josemigallas deleted the portafly/THREESCALE-4688-localization-framework-react-i18next branch April 1, 2020 09:55
@didierofrivia
Copy link
Member

It was a good PR in the end! Congrats, good joint effort! 🎖 🎖

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants